Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UIBULKED-605 Enabling Confirm changes button based on forms state #679

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

vashjs
Copy link
Contributor

@vashjs vashjs commented Jan 28, 2025

Based on the new requirements, it is necessary to activate/deactivate the "Confirm changes" button based on the state of the forms.

Previously, a scenario was possible in which the user filled out one of the forms completely, and one partially. In this situation, the frontend made it possible to submit forms, but cut off partially filled forms and send default values.

After this PR is merged, the "Confirm changes" button will be enabled only in 3 cases:

  • both forms are valid
  • the marc form is valid and the administrative form is pristine
  • the administrative form is valid and the marc form is pristine

Details:

image

Refs: UIBULKED-605

Copy link

github-actions bot commented Jan 28, 2025

Jest Unit Test Statistics

    1 files  ±0    53 suites  ±0   3m 13s ⏱️ -13s
332 tests ±0  332 ✔️ ±0  0 💤 ±0  0 ±0 
339 runs  ±0  339 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit f03377c. ± Comparison against base commit 38c1f8c.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 28, 2025

BigTest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit f03377c. ± Comparison against base commit 38c1f8c.

♻️ This comment has been updated with latest results.

@vashjs vashjs changed the title UIBULKED-605 Enabling Confirm changes button UIBULKED-605 Enabling Confirm changes button based on forms state Jan 28, 2025
@vashjs vashjs marked this pull request as ready for review January 28, 2025 12:10
@vashjs vashjs requested review from UladzislauKutarkin and a team January 28, 2025 12:10
const isAdministrativeFormPristine = !isEqual(ADMINISTRATIVE_FORM_INITIAL_STATE, contentUpdates);
const isMarcFormPristine = !isEqual(MARC_FORM_INITIAL_STATE, marcContentUpdatesWithoutId);

// state is valid if at least one form is filled and valid and another one is not pristine or both forms are filled and valid

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to say in the comment that another one is pristine

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @BogdanDenis here, but I feel like both the comment AND the conditional contradict the PR description which states "2 valid forms, or 1 valid and one untouched". Here, you instead check 2 valid forms or 1 valid and one NOT pristine. Or do you have the booleans on lines 57 and 58 configured backwards, i.e. isPristine should be true when those values are equal, instead of when they differ?

Super petty: since your comment describes two different scenarios, just put them on two different lines. As-is, it's hard to parse how the ands and ors separate things.

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel like the PR description and the code are aligned. Can you confirm?

const isAdministrativeFormPristine = !isEqual(ADMINISTRATIVE_FORM_INITIAL_STATE, contentUpdates);
const isMarcFormPristine = !isEqual(MARC_FORM_INITIAL_STATE, marcContentUpdatesWithoutId);

// state is valid if at least one form is filled and valid and another one is not pristine or both forms are filled and valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with @BogdanDenis here, but I feel like both the comment AND the conditional contradict the PR description which states "2 valid forms, or 1 valid and one untouched". Here, you instead check 2 valid forms or 1 valid and one NOT pristine. Or do you have the booleans on lines 57 and 58 configured backwards, i.e. isPristine should be true when those values are equal, instead of when they differ?

Super petty: since your comment describes two different scenarios, just put them on two different lines. As-is, it's hard to parse how the ands and ors separate things.

@vashjs
Copy link
Contributor Author

vashjs commented Jan 28, 2025

@zburke @BogdanDenis the variables actually had unnecessary inversions of values. I updated the PR description, and divided the variables into more meaningful parts. Please double check.

@vashjs vashjs requested a review from zburke January 28, 2025 15:22
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much clearer. Thank you! I think the comment is still not quite right, but I'll let you decide.

@vashjs vashjs merged commit 7a06ad8 into master Jan 28, 2025
6 checks passed
@vashjs vashjs deleted the UIBULKED-605 branch January 28, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants